Repository navigation
Conversation
update_generated_line_and_column_slow advances `i` past the decoded character before matching on it, so the '\r' arm was inspecting the byte two positions after the carriage return instead of the one right after it. A lone '\r' followed by a one-character line was not counted as a line break, and a real "\r\n" was counted as two, so every mapping after it landed on the wrong generated line. Peek at slice[i], which is the byte following the '\r'.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 5c7d4c1 has some failures in 🧪 To try this PR locally: bunx bun-pr 38449That installs a local version of the PR into your bun-38449 --bun |
|
Status: reproduced on 1.4.0 and main with the commands in the PR body ( |
There was a problem hiding this comment.
LGTM — the CRLF peek was checking two bytes past the CR because i had already been advanced; peeking at slice[i] is the correct byte-after-CR.
What was reviewed:
- Traced the loop:
i += lenruns before the match, soslice[i]is the byte after\r; thei < nbound covers CR-at-end. - Checked the two sibling CRLF loops in
src/sourcemap/lib.rsandLineOffsetTable.rs— both index the rune itself pre-advance, so neither shares this bug. - Tests cover both failure directions (lone CR under-counted, CRLF double-counted) through both the VLQ bundler path and the runtime internal-map path;
readFileSyncsidestepsapi.readFile()'s CRLF normalization.
Extended reasoning...
Overview
One-line fix in src/sourcemap/Chunk.rs::update_generated_line_and_column_slow: the \r arm now peeks at slice[i] (the byte immediately after the CR) instead of output[last_generated_update + i + 1] (two bytes past it, since i is post-advanced by i += len before the match). The old expression was a straight port from esbuild's loop where i indexes the rune itself. Five new tests across test/bundler/bundler_comments.test.ts and test/js/bun/sourcemap/internal-sourcemap.test.ts pin both failure modes.
Security risks
None. This is line-counting logic for sourcemap generation over already-printed output; no user-controlled sizes drive allocation, and the new i < n bound guards the only added indexed read.
Level of scrutiny
Low-to-medium. The native change is a single condition whose correctness is verifiable by tracing the loop locally, and the PR body derives the exact off-by-one from the esbuild reference. I confirmed the two other CRLF-peek sites in src/sourcemap/ (lib.rs:193-195 and LineOffsetTable.rs:256) use pre-advance indexing and are already correct, so there are no unfixed siblings.
Other factors
The tests are well-constructed against the review guide: they assert exact original-line values via SourceMap.findEntry / error.stack rather than string shape, cover both the bundler VLQ format and the runtime internal format (both flow through this one loop), read the outfile with readFileSync to avoid the harness's CRLF normalization, and exercise both the lone-CR-followed-by-one-char and the actual-CRLF shapes that the old code got wrong in opposite directions. robobun confirmed all five fail on the unfixed build.
Problem
bun build --sourcemapoutput and the maps the runtime uses to remaperror.stack: a lone\rfollowed by a one-character line is not counted as a line break, and an actual\r\nis counted as two./*! a\rb\nc */\nconsole.log(1);\nthrow new Error("x");\nwith--sourcemap=external. The output putsconsole.logon its 5th line (the\rends a line, as it does for every JS engine), butmappingsis;AAGA;AAAA;AAAA,QAAQ,IAAI,CAAC;..., withconsole.login the 4th line group. Running a file with that comment reports anew Error()created on line 4 as line 5.update_generated_line_and_column_slowinsrc/sourcemap/Chunk.rsadvancesipast the decoded character before matching on it, but the\rarm peeked atlast_generated_update + i + 1, which is two bytes past the CR. (The+ 1is from esbuild's version of this loop, whereiis the index of the rune itself.) So\rX\nskips the CR because the byte two ahead is a\n, and\r\nXcounts the CR and then the\nagain.Fix
\rarm peeks atslice[i], the byte directly after the CR. That condition is the whole change; the rest of the hunk is the comment.\r,\n, U+2028 and U+2029 each end a line,\r\nends one), and it is the ruleLineOffsetTable::generatealready applies to the original side of every mapping, so the two sides of a mapping now count lines the same way.\r\nto\ninside legal comments in the printer, which left the builder wrong for the CRs that still get through (the shapes tested here). Fixing the peek where the count happens covers those without more printer-side normalization.test/bundler/bundler_comments.test.ts: three bundles (lone CR in a legal comment, CRLF left in a legal comment, CRLF in an inlined enum member comment) check that the generated line holding each statement after the comment maps back to that statement's line in the entry point. Unfixed, the statement after the lone CR maps to the line below it and the one after the CRLF maps to the line above it.test/js/bun/sourcemap/internal-sourcemap.test.ts: the same two shapes throughbun runanderror.stack, which go through the same builder but the runtime's internal map format. Unfixed, they report line 5 instead of 4 and line 4 instead of 5.test/js/bun/sourcemap/,bundler_edgecase, the compile source map tests and thenode:modulesource map tests against the debug build.Background
update_generated_line_and_columnscans whatever was printed since the previous call to advance the generated line and column, emitting one line separator (a;in VLQ maps, a separator entry in the runtime's internal format) per line break it sees.bun buildmaps and the runtime's stack trace maps both come out of this one loop./*! ... */) are copied verbatim except that a\r\ninside them becomes\n, so a lone\rsurvives and\r\r\ncomes out as a real\r\n. Accessing an inlined enum member prints1 /* member name */with the name verbatim. Strings and template literals escape or normalize CRs, so the tests use those two shapes.LineOffsetTable::generate) steps over the declared width of an invalid UTF-8 lead byte, so a Latin-1 byte directly before a newline still swallows that line break. Different cause, tracked separately.Repro output before and after